Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Nov 2, 2024

I was planning on removing a bunch of zval_get_long() and zval_get_string() calls to properties as those are typed properties, however not sure it is wise to remove them as ArrayObject can in theory mess them up.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, only some remark about comments

if (file) {
if (Z_TYPE_P(file) != IS_STRING) {
if (UNEXPECTED(Z_TYPE_P(file) != IS_STRING)) {
/* This is a typed property and can only happen if modified via ArrayObject */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true, see bug63762.phpt in Zend

if (EXPECTED(Z_TYPE_P(tmp) == IS_LONG)) {
line = Z_LVAL_P(tmp);
} else {
/* This is a typed property and can only happen if modified via ArrayObject */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely not true as well due to similar reasons as bug63762.phpt in Zend

ZSTR_LEN(str->s) -= 2; /* remove last ', ' */
}
} else {
/* The trace property is typed and private */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check with bug63762.phpt in Zend

@Girgias Girgias merged commit 23b8d64 into php:master Nov 10, 2024
10 checks passed
@Girgias Girgias deleted the exceptions-refacto branch November 10, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants